Conversation
Summary of ChangesHello @dioo1461, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 풀리퀘스트는 사용자별 그룹 색상 할당 기능을 도입하여 그룹 관리의 개인화를 강화합니다. 이를 위해 새로운 React 훅을 추가하고, 기존 그룹 관련 컴포넌트들을 리팩토링했습니다. 또한, 학습 시간 기록 로직의 안정성을 개선하고, 비동기 저장소 키 관리 방식을 업데이트하여 데이터의 정확성과 사용자 경험을 향상시켰습니다. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| await storeChats(entities); | ||
| const recentChatId = fetchedChats[fetchedChats.length - 1].chatId; | ||
| asyncStorage.set(ASYNC_KEYS.groupChatCursor(chatRoomId), recentChatId); | ||
| asyncStorage.set(ASYNC_KEYS.groupChatCursor(myUserId, chatRoomId), recentChatId); |
There was a problem hiding this comment.
"groupChatCursor" 키에 "myUserId"를 포함하도록 변경한 것은 매우 중요합니다. 이는 각 사용자의 채팅 커서가 독립적으로 저장되도록 보장하여, 여러 사용자가 동일한 기기에서 앱을 사용하거나 사용자 계정이 전환될 때 발생할 수 있는 데이터 충돌을 방지하는 치명적인 수정입니다.
| groupChatCursor: (userId: number, chatRoomId: string) => `groupChat:${userId}-${chatRoomId}`, | ||
| groupColor: (userId: number, groupId: number) => `groupColor:${userId}-${groupId}`, |
There was a problem hiding this comment.
"groupChatCursor" 키에 "userId"를 포함하도록 수정한 것은 채팅 커서에 대한 데이터 무결성을 보장하는 치명적인 개선입니다. 또한, "groupColor" 키를 "userId"와 "groupId"를 모두 포함하도록 추가한 것은 새로운 그룹 색상 할당 기능을 올바르게 저장하고 검색하는 데 필수적입니다.
|
|
||
| await storeChats([chatObjToEntity(newChat, chatRoomId)]); | ||
| await asyncStorage.set(ASYNC_KEYS.groupChatCursor(chatRoomId), message.id); | ||
| await asyncStorage.set(ASYNC_KEYS.groupChatCursor(myUserId, chatRoomId), message.id); |
There was a problem hiding this comment.
"groupChatCursor" 키에 "myUserId"를 포함하도록 변경한 것은 "useGroupChatBootstrap.ts"와 동일하게 사용자별 데이터 격리를 위한 치명적인 수정입니다.
|
|
||
| const cursor = | ||
| (await asyncStorage.get(ASYNC_KEYS.groupChatCursor(chatRoomId))) ?? undefined; | ||
| (await asyncStorage.get(ASYNC_KEYS.groupChatCursor(myUserId, chatRoomId))) ?? undefined; |
There was a problem hiding this comment.
"groupChatCursor" 키에 "myUserId"를 포함하도록 변경한 것은 매우 중요합니다. 이는 각 사용자의 채팅 커서가 독립적으로 저장되도록 보장하여, 여러 사용자가 동일한 기기에서 앱을 사용하거나 사용자 계정이 전환될 때 발생할 수 있는 데이터 충돌을 방지하는 치명적인 수정입니다.
|
|
||
| const GroupDetailScreen = () => { | ||
| const groupColor = GROUP_COLORS[theme.color.blue[300]]; | ||
| const groupColor = GROUP_COLORS[useStackRoute<'GroupDetail'>().params.groupColorKey]; |
There was a problem hiding this comment.
"groupColor"를 하드코딩된 파란색 대신 "useStackRoute().params.groupColorKey"에서 동적으로 가져오도록 변경한 것은 그룹별 색상 할당 기능의 핵심적인 부분입니다. 이는 그룹 상세 화면이 할당된 그룹 색상을 올바르게 반영하도록 보장합니다.
| <StackableScreenLayout | ||
| backgroundColor={GROUP_COLORS[groupColorKey].weak} | ||
| headerElementColor={GROUP_COLORS[groupColorKey].medium} | ||
| headerElementColor={GROUP_COLORS[groupColorKey].strong} |
There was a problem hiding this comment.
"headerElementColor"를 "GROUP_COLORS[groupColorKey].strong"으로 변경하여 그룹 색상 테마의 'strong' 변형을 사용하도록 한 것은 UI의 시각적 일관성을 높입니다.
| header, | ||
| children, | ||
| safeAreas, | ||
| safeAreas = 'top', |
There was a problem hiding this comment.
"safeAreas" prop에 "top"을 기본값으로 설정하여 컴포넌트 사용 시 명시적으로 지정하지 않아도 안전 영역 처리가 되도록 개선했습니다. 이는 컴포넌트의 사용 편의성을 높입니다.
| backgroundColor={groupColor.weak} | ||
| headerElementColor={groupColor.strong} |
There was a problem hiding this comment.
하드코딩된 색상 대신 "groupColor.weak" 및 "groupColor.strong"을 사용하여 배경 및 헤더 요소 색상을 동적으로 설정한 것은 그룹별 색상 할당 기능과의 일관성을 높이는 좋은 변경입니다.
| if (diff < 0) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn('Resulting time is negative. Returning zero time.'); | ||
| console.log('[Time] Resulting time is negative. Returning zero time.'); |
There was a problem hiding this comment.
음수 시간 결과에 대한 로깅을 "console.warn"에서 "console.log"로 변경한 것은 해당 상황이 경고 수준의 문제가 아니라 정보성 메시지임을 나타냅니다. 이는 로깅의 적절성을 개선합니다.
| } | ||
|
|
||
| const recordedStudyTime = todayRecord.time; | ||
| const recordedStudyTime = todayRecord?.time ?? Time.fromMilliseconds(0); |
There was a problem hiding this comment.
"StudyDataHydrator.tsx"와 마찬가지로 "todayRecord"가 "null"일 때 "console.error" 대신 "?? Time.fromMilliseconds(0)"를 사용하여 기본값을 할당하는 것은 일관성 있고 안정적인 오류 처리 방식입니다.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ecdb44ee2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| loadMoreChats, | ||
| storeChats, | ||
| setChats, | ||
| myUserId: profileData?.id ?? null, | ||
| myUserId: profileData?.id ?? -1, | ||
| sendReadCursor, |
There was a problem hiding this comment.
Using profileData?.id ?? -1 means useGroupChatBootstrap runs before the real profile id is loaded. In that first pass it fetches and appends chats, and stores the cursor under groupChat:-1-…. When the profile data arrives, the effect re-runs with the real id, reads an empty cursor (different key) and fetches the same history again, appending duplicates to chats. This happens any time the profile query resolves after the chat screen mounts. Consider keeping myUserId as null until the profile is ready, or guard the bootstrap hook so it only runs when a valid user id is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR implements a feature to assign random colors to groups on a per-user basis, ensuring each user sees groups with consistent but personalized colors.
Changes:
- Added
useGroupColorKeyhook that stores and retrieves user-specific group colors from async storage - Updated async storage keys to include user ID for per-user data isolation
- Changed chat-related hooks to use
-1instead ofnullas a fallback for missing user IDs - Refactored error handling in study time tracking to use default values instead of logging errors
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/features/groups/hooks/useGroupColorKey.ts | New hook that generates and persists random group colors per user |
| src/utils/asyncStorage/index.ts | Added user ID to storage keys for groupChatCursor and groupColor |
| src/screens/bottomTab/GroupsScreen/components/JoinedGroupPanel/index.tsx | Refactored to use new useGroupColorKey hook with GroupListItem component |
| src/screens/group/groupDetail/GroupDetailScreen/index.tsx | Updated to use groupColorKey from route params and apply colors to layout |
| src/screens/group/GroupSettingScreen/index.tsx | Changed header element color from medium to strong |
| src/screens/group/GroupRankingScreen/index.tsx | Changed header element color from medium to strong |
| src/screens/group/GroupChatScreen/index.tsx | Changed userId fallback from null to -1 |
| src/screens/group/GroupChatScreen/hooks/useGroupChatRealtime.ts | Updated myUserId type from number | null to number |
| src/screens/group/GroupChatScreen/hooks/useGroupChatBootstrap.ts | Updated to use new groupChatCursor key with userId parameter |
| src/features/study/api/mutations.ts | Removed error logging and used default Time value when record is missing |
| src/StudyDataHydrator.tsx | Removed error logging and used default Time value when record is missing |
| src/layout/StackableScreenLayout/index.tsx | Added default value 'top' for safeAreas prop |
| src/utils/Time/index.ts | Changed console.warn to console.log for negative time warning |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (async () => { | ||
| const stored = await asyncStorage.get(ASYNC_KEYS.groupColor(userId, groupId)); | ||
| if (stored && isGroupColorKey(stored)) { | ||
| if (!cancelled) setColorKey(stored); | ||
| return; | ||
| } | ||
| const newKey = pickRandomGroupColorKey(); | ||
| await asyncStorage.set(ASYNC_KEYS.groupColor(userId, groupId), newKey); | ||
| if (!cancelled) setColorKey(newKey); |
There was a problem hiding this comment.
The cleanup function sets cancelled = true after the async operation has started, but there's a potential race condition. If the component unmounts while the async operation is in progress (between lines 25-32), setColorKey could still be called after the component has unmounted if the check if (!cancelled) hasn't been reached yet. The cancellation flag should be checked before each state update, but line 32 only checks it once. Consider checking cancelled before line 27 as well to prevent state updates on unmounted components.
| const GroupListItem = ({ group }: { group: GetGroupsResponse['data'][number] }) => { | ||
| const groupColorKey = useGroupColorKey(group.groupId); | ||
| if (!groupColorKey) return null; |
There was a problem hiding this comment.
The hook returns null when groupColorKey is null, causing the GroupListItem component to return null and not render. This creates an inconsistent user experience where groups may temporarily not appear in the list while their color is being loaded from async storage. Consider providing a default color or loading state instead, or ensure that the color is loaded before rendering the group list.
| const groupColor = GROUP_COLORS[useStackRoute<'GroupDetail'>().params.groupColorKey]; | ||
|
|
||
| const { data: categories } = useCategoriesQuery(); | ||
| const { params } = useStackRoute<'GroupDetail'>(); |
There was a problem hiding this comment.
The useStackRoute hook is called twice on lines 19 and 22, which is redundant and violates the React principle of calling hooks in a consistent order. The first call on line 19 is only used to access params.groupColorKey, while the second call is destructured to get params. Consider consolidating these into a single call by storing the result in a variable and accessing both properties from it.
| const groupColor = GROUP_COLORS[useStackRoute<'GroupDetail'>().params.groupColorKey]; | |
| const { data: categories } = useCategoriesQuery(); | |
| const { params } = useStackRoute<'GroupDetail'>(); | |
| const { params } = useStackRoute<'GroupDetail'>(); | |
| const groupColor = GROUP_COLORS[params.groupColorKey]; | |
| const { data: categories } = useCategoriesQuery(); |
| @@ -1,4 +1,4 @@ | |||
| import { useFocusEffect } from '@react-navigation/native'; | |||
| import { useFocusEffect, useRoute } from '@react-navigation/native'; | |||
There was a problem hiding this comment.
The useRoute import is unused after the refactoring. This import should be removed.
| import { useFocusEffect, useRoute } from '@react-navigation/native'; | |
| import { useFocusEffect } from '@react-navigation/native'; |
| isConnected: boolean; | ||
| chatRoomId: string; | ||
| myUserId: number | null; | ||
| myUserId: number; |
There was a problem hiding this comment.
The type of myUserId was changed from number | null to number. However, this change may cause type inconsistencies with other parts of the system. The useChatDB hook still expects userId: number | null, and the calling code in GroupChatScreen passes profileData?.id ?? -1 as a fallback. It appears -1 is being used as a sentinel value for missing user IDs, but this is not documented. Consider using a consistent approach - either update all related types to use -1 as a sentinel value with proper documentation, or keep using null to represent missing user IDs.
| myUserId: number; | |
| myUserId: number | null; |
| export const ASYNC_KEYS = { | ||
| language: 'language', | ||
| groupChatCursor: (chatRoomId: string) => `groupChat:${chatRoomId}`, | ||
| groupChatCursor: (userId: number, chatRoomId: string) => `groupChat:${userId}-${chatRoomId}`, |
There was a problem hiding this comment.
The signature of groupChatCursor was changed to include userId as the first parameter, but existing stored keys from the previous version will not be accessible anymore (keys were previously groupChat:${chatRoomId}, now groupChat:${userId}-${chatRoomId}). This is a breaking change that will lose chat cursor data for existing users. Consider implementing a migration strategy or documenting this as an intentional data reset.
| if (diff < 0) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn('Resulting time is negative. Returning zero time.'); | ||
| console.log('[Time] Resulting time is negative. Returning zero time.'); |
There was a problem hiding this comment.
Changing from console.warn to console.log reduces the visibility of this edge case (negative time differences). In the codebase, console.warn is consistently used for warning scenarios (see src/features/chat/sse/messageParsers.ts:23, src/features/chat/sse/messageParsers.ts:36, src/features/chat/sse/messageParsers.ts:45, src/hooks/useStompConnection/index.ts:15, src/utils/fetch/executeFetch.ts:62). Consider keeping console.warn for consistency and better visibility of this edge case.
dioo1461
force-pushed
the
feature/group-color-management
branch
from
5ecdb44 to
5bb0b2d
Compare
✨ 구현 기능 명세
✅ PR Point
😭 어려웠던 점